Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a feature to allow blue team members to view non-anonymized team names on score graphs, addressing issue #125. It adds a new configuration option AllowNonAnonymizedGraphsForBlueTeam that controls whether team names are displayed or anonymized to "Team" for non-admin users.
Changes:
- Added new configuration field
AllowNonAnonymizedGraphsForBlueTeamto control graph anonymization - Refactored duplicate anonymization logic into a shared
shouldScrubfunction - Extended anonymization logic to the
GetServiceStatusendpoint for consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| engine/config/config.go | Adds new boolean config field AllowNonAnonymizedGraphsForBlueTeam to UIConfig struct |
| www/api/graphs.go | Refactors anonymization logic into shouldScrub helper function and applies it consistently across all three graph endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if shouldScrub(r) { | ||
| for i := range series { | ||
| series[i].Name = "Team" |
There was a problem hiding this comment.
When scrubbing team names, the team ID is still exposed in the Series struct. This defeats the purpose of anonymization since clients can still identify teams by their IDs. Consider also scrubbing or removing the ID field when shouldScrub returns true, similar to how the Name field is scrubbed.
| series[i].Name = "Team" | |
| series[i].Name = "Team" | |
| series[i].ID = 0 |
| if r.Context().Value("roles") != nil { | ||
| req_roles := r.Context().Value("roles").([]string) | ||
| if slices.Contains(req_roles, "admin") { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| if conf.UISettings.AllowNonAnonymizedGraphsForBlueTeam { | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of AllowNonAnonymizedGraphsForBlueTeam affects all non-admin users, including red team members and anonymous users. Based on the config name and the issue description ("Add team numbers to score graphs for blue teams"), this setting should likely only apply to users with the "team" role. Consider checking for the "team" role before returning false here. For example: if the config is enabled AND the user has the "team" role, then don't scrub. This would prevent red team members and anonymous users from seeing team identities when they shouldn't.
| if r.Context().Value("roles") != nil { | |
| req_roles := r.Context().Value("roles").([]string) | |
| if slices.Contains(req_roles, "admin") { | |
| return false | |
| } | |
| } | |
| if conf.UISettings.AllowNonAnonymizedGraphsForBlueTeam { | |
| return false | |
| } | |
| rolesVal := r.Context().Value("roles") | |
| if rolesVal != nil { | |
| if reqRoles, ok := rolesVal.([]string); ok { | |
| if slices.Contains(reqRoles, "admin") { | |
| return false | |
| } | |
| if conf.UISettings.AllowNonAnonymizedGraphsForBlueTeam && slices.Contains(reqRoles, "team") { | |
| return false | |
| } | |
| } | |
| } |
1 participant
Fixes #125